Fixes state writes with private variables #504
Conversation
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to 8a9c3c5 in 27 seconds
More details
- Looked at
70lines of code in2files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. burr/core/application.py:194
- Draft comment:
The parameterstate_subset_pre_updatein the docstring does not match the actual parameter namestate_to_modify. Please update the docstring to reflect the correct parameter name. - Reason this comment was not posted:
Comment was not on a valid diff hunk.
Workflow ID: wflow_tax4fUCpzioXToNM
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
|
A preview of 00f17f4 is uploaded and can be seen here: ✨ https://burr.dagworks.io/pull/504 ✨ Changes may take a few minutes to propagate. Since this is a preview of production, content with |
This drops private variables when written by an action. Note we do *not* add tests for this as it is undefined behavior -- we may error out later on.
elijahbenizzy
force-pushed
the
fix-state-write-underscores
branch
from
8a9c3c5 to
00f17f4
Compare
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 00f17f4 in 30 seconds
More details
- Looked at
70lines of code in2files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. docs/concepts/state.rst:33
- Draft comment:
The documentation should mention that private variables (starting with__) are automatically dropped during state updates to prevent undefined behavior. - Reason this comment was not posted:
Comment was on unchanged code.
2. burr/core/application.py:194
- Draft comment:
The comment mentions "undefined behavior" but does not specify that private variables are dropped during state updates. Consider clarifying this in the comment. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_Q4ZdyXPDmeLSVdSk
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Yes, it's specifically undefined (as in we want to address it further but not now), so I'm comfortable not adding tests for now as to not encode a contract we don't need. |
This drops private variables when written by an action. Note we do not add tests for this as it is undefined behavior -- we may error out later on.
Important
_state_updateinapplication.pynow drops private variables when written by an action, with a warning added instate.rstabout undefined behavior._state_updateinapplication.pynow drops private variables (keys starting with__) when written by an action.state.rstabout undefined behavior when modifying private variables starting with__.This description was created by
for 00f17f4. It will automatically update as commits are pushed.